Add a header type field #4993

tyrannosaurus-becks · 2018-07-25T20:11:20Z

This field type came out of discussion here.

This adds a header type field. Under the hood, that's a map[string][]string which makes sense since one header key can have multiple values.

Once it's added, I can immediately use it in Alibaba creds (currently in development), and we could optionally retrofit AWS creds as well.

Here's how this field feels from the CLI:

vault write auth/somewhere/foo headers='hello:world'
vault write auth/somewhere/foo headers='hello:world' headers='fizz:buzz'
vault write auth/somewhere/foo headers='hello:world' headers='hello:monde'
vault write auth/somewhere/foo headers='eyJDb250ZW50LUxlbmd0aCI6IFsiNDMiXSwgIlVzZXItQWdlbnQiOiBbImF3cy1zZGstZ28vMS40LjEyIChnbzEuNy4xOyBsaW51eDsgYW1kNjQpIl0sICJYLVZhdWx0LUFXU0lBTS1TZXJ2ZXItSWQiOiBbInZhdWx0LmV4YW1wbGUuY29tIl0sICJYLUFtei1EYXRlIjogWyIyMDE2MDkzMFQwNDMxMjFaIl0sICJDb250ZW50LVR5cGUiOiBbImFwcGxpY2F0aW9uL3gtd3d3LWZvcm0tdXJsZW5jb2RlZDsgY2hhcnNldD11dGYtOCJdLCAiQXV0aG9yaXphdGlvbiI6IFsiQVdTNC1ITUFDLVNIQTI1NiBDcmVkZW50aWFsPWZvby8yMDE2MDkzMC91cy1lYXN0LTEvc3RzL2F3czRfcmVxdWVzdCwgU2lnbmVkSGVhZGVycz1jb250ZW50LWxlbmd0aDtjb250ZW50LXR5cGU7aG9zdDt4LWFtei1kYXRlO3gtdmF1bHQtc2VydmVyLCBTaWduYXR1cmU9YTY5ZmQ3NTBhMzQ0NWM0ZTU1M2UxYjNlNzlkM2RhOTBlZWY1NDA0N2YxZWI0ZWZlOGZmYmM5YzQyOGMyNjU1YiJdfQ=='
vault write auth/somewhere/foo headers='{"hello":"world","bonjour":["monde","dieu"]}' (with proper bash escaping)

That's for a schema field named headers that is of TypeHeader.

chrishoffman · 2018-07-27T12:47:01Z

logical/framework/field_data_test.go

+				"foo": {Type: TypeHeader},
+			},
+			map[string]interface{}{
+				"foo": []interface{}{"key1=value1", "key2=value2", "key3=1"},


What happens if a header key name is repeated? I would expect it to end up with a list of values but I don't see a test for that.

Additionally, if you provide a single value, I believe it will come in as a string interface and not a list. See TypeCommaStringSlice.

Thanks! I'll check out TypeCommaStringSlice.

On the double-key thing, I didn't add a test case for that because of the format of the test cases being a map, which obviously doesn't allow the same key multiple times. I'll break out a separate test for that.

But I would expect CLI params of key1=value1 key1=value2 to result in:

map[string]interface{}{ "key1": []interface{}{"value1", "value2"} }

... or something like that :).

Oh I see what you're saying. 👍

chrishoffman · 2018-07-27T12:49:07Z

logical/framework/field_data_test.go

+				"foo": {Type: TypeHeader},
+			},
+			map[string]interface{}{
+				"foo": map[string][]string{


I think the JSON Unmarshaller will return this as a map[string][]interface{}, can you test some non-string value and make sure you get what you expect.

It was a good test!

I ran Vault in dev mode and made a little path that took TypeHeader for a field named headers. Then I used the following command:

vault write auth/somewhere/foo headers='hello:world' headers='hello:true' headers="fizz:1"

And it actually parsed both the faux bool and the faux int field with no error, and they came in as strings, which is what I would want for a header.

chrishoffman · 2018-07-27T12:54:47Z

logical/framework/field_data.go

+			// canonicalize all the keys.
+			result := http.Header{}
+			for k, slice := range mapResult {
+				canonicalKey := textproto.CanonicalMIMEHeaderKey(k)


Do you need to do this step? It seems to me you would want to preserve formatting and any comparisons you would retrieve the header using the Get function and do any normalized text compares there.

It's definitely weird.

The reason I do it is because if you send in a header of "hello:world" and then later do header.Get("hello"), it doesn't find "world". That's because under the hood, the Get method first converts "hello" to "Hello" and then looks for it in the map.

I thought this was a really weird behavior, and tried to think through how to best achieve what someone would expect. I thought about, instead of using an http.Header, maybe I should just use a straight-out map. But then, people are sending in headers so maybe they would expect that kind of behavior.

In the end, I did this because I thought from a usability perspective it might be the best option. But I'm open to other approaches because it's a tough call.

I am relatively certain that colons are illegal in header key values, which would explain this behavior.

I don't know if I explained that well LOL. In the example of "hello:world" I was thinking of "hello" as the key and "world" as the value. Probably would have been a better example if I'd done JSON or something.

I was trying to say that if you give it a lower-case key of hello, then later try to pull out a lower-case hello, it finds nothing. And it's because under the hood, it converts it to Title Case before searching for it in the map, so it's looking for "Hello" while it's sitting in the map as "hello", and thus misses it. It converts it to Title Case using that canonical method.

So by also calling that canonical method on keys on the way in, it also Title Cases the keys in the map so they'll have hits. This is definitely a trade-off because, while you'll now have case-insensitive hits using the header.Get("hello") method, if you need to pull headers out like this: header["hello"] it won't work anymore. You'd use the Get method for headers that you know only have one value, and the map-key-accessing method for headers that may have multiple values.

Ultimately, I'm honestly not super in love with the magic the http.Header type is doing because I think it's actually pretty weird. This is why I'm tempted to just turn it into a map[string][]string, and to do away with the canonical stuff, so it's way more evident and straight-forward how to use it.

I too struggled with this type for whatever reason, it is challenging to use. Maybe you're doing a little too much sanitation and you just need to pass through what they give you and serialize it directly to disk when necessary. Here is the code I came up with dealing with headers, config, and output. https://github.com/hashicorp/vault/blob/master/vault/ui.go

Probably you shouldn't do any sanitization at all -- you should get it to the correct type and just copy it over verbatim. There's no need for you to do any textproto stuff when Header is doing it anyways.

jefferai · 2018-07-27T15:49:05Z

I don't know if there are any characters that are not valid in a header value, but if so it would be cool to be able to parse a pure string too. Thinking about thngs like #4982; this could move to parseutil (it probably should anyways) and be accessible from the API/CLI.

tyrannosaurus-becks · 2018-07-27T17:38:26Z

@jefferai I read through #4982 and I'm not sure what you mean. Can you clarify?

Are you thinking it would be cool to also parse the headers to validate them? If so I could add logic mirroring RFC 7230.

jefferai · 2018-07-27T17:41:13Z

Ultimately, I'm honestly not super in love with the magic the http.Header type is doing because I think it's actually pretty weird. This is why I'm tempted to just turn it into a map[string][]string, and to do away with the canonical stuff, so it's way more evident and straight-forward how to use it.

It does magic to work around the exact kind of issue you're running into by trying to access the map directly. Why not just always use the http.Header methods?

jefferai · 2018-07-27T17:44:06Z

I read through #4982 and I'm not sure what you mean. Can you clarify?

What I mean is that the processing logic, if in parseutil, makes it easier to potentially share with other needs, such as if we build in arbitrary header support into CLI/API for requests. That's why we moved various other processing bits like TypeDurationSecond into parseutil.

Are you thinking it would be cool to also parse the headers to validate them? If so I could add logic mirroring RFC 7230.

No, I think using http.Header validates them for you. And if not, I'm fine with "buyer beware"

tyrannosaurus-becks · 2018-07-27T18:02:34Z

Why not just always use the http.Header methods?

I think this is a waaaay better explanation of what happens using the http.Header methods. IMHO the problem is that it's missing any method for getting an array of values that handles using the correctly-cased header key. Another option might even be writing a wrapper for http.Header that simply adds a method like that.

This might better illustrate what I'm getting at: https://play.golang.org/p/ohI4WuIEwUE

jefferai · 2018-07-27T22:14:34Z

If you're accessing the map directly just use textproto to canonicalize it, problem solved!

tyrannosaurus-becks · 2018-07-27T22:39:10Z

@jefferai I totally agree that's how people should do it, but would they know to, or would it lead to bugs? Likewise, would they realize that under the hood it's a map[string][]string and they should look for all values and not just use the Get method?

Just want to have the discussion before we add this type and it gets reused in multiple places.

jefferai · 2018-07-27T23:05:57Z

I guess I'm not really following the problem. If you want to provide utility functions I suggest creating a new type that embeds an http.Header, that seems useful.

jefferai · 2018-07-30T20:42:04Z

logical/framework/field_type.go

+	To avoid bugs like this, we provide one more method. GetAll,
+	which returns all the values for a key.
+*/
+func NewHeader() Header {


This should probably all go in parseutil if ParseHeader is in there (which it may be?)

Np, I'll move it!

tyrannosaurus-becks · 2018-08-07T22:01:19Z

Chatted with a couple folks about this, and we agreed we'd be OK with the risk of just using a raw http.Header field, so I've turned this back to that. Should be good to go!

jefferai · 2018-08-13T17:49:14Z

logical/framework/field_data.go

+				case string:
+					header.Add(headerKey, typedHeader)
+				case []string:
+					for _, headerVal := range typedHeader {


Doesn't this need to be range typedHeader.([]string)? Same with the case below.

Yes and no. It's already converted to an []string here: switch typedHeader := headerValGroup.(type). The type is used in the switch below and it's actually cast to it when it's instantiated there on the left. Went off of this.

Ah right, I mixed up:

switch v.(type) {

which doesn't have that type with what you did here, which does. Great!

jefferai

I think it looks good, although I'm surprised nothing is complaining when you're ranging over an interface{}.

add a header type field

7b86f41

tyrannosaurus-becks requested a review from jefferai July 25, 2018 20:12

tyrannosaurus-becks mentioned this pull request Jul 25, 2018

Add Alibaba auth method hashicorp/vault-plugin-auth-alicloud#8

Merged

tyrannosaurus-becks added this to the 0.11 milestone Jul 25, 2018

chrishoffman reviewed Jul 27, 2018

View reviewed changes

chris feedback changes

fdbb32a

Becca Petrin added 2 commits July 27, 2018 14:59

strip MIME conversion as Add does it

7ad2511

Merge branch 'opensource-master' into add-type-headers

bc18aa0

Becca Petrin added 2 commits July 30, 2018 13:33

add wrapper for http header

a3a7cb3

update comment

a3286a9

jefferai reviewed Jul 30, 2018

View reviewed changes

Becca Petrin added 2 commits July 30, 2018 13:53

strip outdated comments

ef89439

move header to parseutil

d4fb65f

tyrannosaurus-becks modified the milestones: 0.11, 0.10.5 Jul 31, 2018

Becca Petrin added 2 commits August 7, 2018 14:58

use http.Header type

f0443e0

Merge branch 'opensource-master' into add-type-headers

b190a4e

Becca Petrin added 3 commits August 9, 2018 13:45

add support for b64-encoded json headers

b1cd520

support headers as json strings or b64d json

ab5a906

fix comment typo

f486653

jefferai reviewed Aug 13, 2018

View reviewed changes

jefferai approved these changes Aug 13, 2018

View reviewed changes

tyrannosaurus-becks merged commit 13a2bb1 into master Aug 13, 2018

tyrannosaurus-becks deleted the add-type-headers branch August 13, 2018 18:02

tyrannosaurus-becks mentioned this pull request Aug 13, 2018

Get TypeHeader field dependency from Vault hashicorp/vault-plugin-auth-alicloud#12

Merged

catsby mentioned this pull request Sep 11, 2018

Update AWS auth backend iam_request_headers to be TypeHeader #5320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a header type field #4993

Add a header type field #4993

tyrannosaurus-becks commented Jul 25, 2018 •

edited

Loading

chrishoffman Jul 27, 2018

chrishoffman Jul 27, 2018

tyrannosaurus-becks Jul 27, 2018

chrishoffman Jul 27, 2018 •

edited

Loading

tyrannosaurus-becks Jul 27, 2018

chrishoffman Jul 27, 2018

tyrannosaurus-becks Jul 27, 2018

tyrannosaurus-becks Jul 27, 2018 •

edited

Loading

chrishoffman Jul 27, 2018

tyrannosaurus-becks Jul 27, 2018

jefferai Jul 27, 2018

tyrannosaurus-becks Jul 27, 2018 •

edited

Loading

chrishoffman Jul 27, 2018

jefferai Jul 27, 2018

jefferai commented Jul 27, 2018

tyrannosaurus-becks commented Jul 27, 2018

jefferai commented Jul 27, 2018

jefferai commented Jul 27, 2018

tyrannosaurus-becks commented Jul 27, 2018 •

edited

Loading

jefferai commented Jul 27, 2018

tyrannosaurus-becks commented Jul 27, 2018 •

edited

Loading

jefferai commented Jul 27, 2018

jefferai Jul 30, 2018

tyrannosaurus-becks Jul 30, 2018

tyrannosaurus-becks commented Aug 7, 2018

jefferai Aug 13, 2018

tyrannosaurus-becks Aug 13, 2018

jefferai Aug 13, 2018

jefferai left a comment

Add a header type field #4993

Add a header type field #4993

Conversation

tyrannosaurus-becks commented Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishoffman Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai commented Jul 27, 2018

tyrannosaurus-becks commented Jul 27, 2018

jefferai commented Jul 27, 2018

jefferai commented Jul 27, 2018

tyrannosaurus-becks commented Jul 27, 2018 • edited Loading

jefferai commented Jul 27, 2018

tyrannosaurus-becks commented Jul 27, 2018 • edited Loading

jefferai commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

tyrannosaurus-becks commented Jul 25, 2018 •

edited

Loading

chrishoffman Jul 27, 2018 •

edited

Loading

tyrannosaurus-becks Jul 27, 2018 •

edited

Loading

tyrannosaurus-becks Jul 27, 2018 •

edited

Loading

tyrannosaurus-becks commented Jul 27, 2018 •

edited

Loading

tyrannosaurus-becks commented Jul 27, 2018 •

edited

Loading